Repository navigation
feat(websocket): flush the log buffer when a server is unreachable - #1132
Merged
Merged
Conversation
An unreachable server keeps retrying with backoff and never reaches a terminal failure reason, so the connection log buffer was never flushed and a user reproducing a "hangs on connecting" problem got no buffered detail in their support bundle. Track consecutive failed connect attempts on ReconnectingWebSocket and, once they reach a threshold (6, ~15s of retrying with the default backoff: past a transient blip, before the 30s cap), flush the buffer once with an "unreachable" reason and emit a connection.unreachable telemetry event. The `=== N` check keeps it to one flush per outage; a successful open resets the counter so a later outage flushes again. Closes #1112.
aqandrew
marked this pull request as ready for review
October 1, 2026 19:40
EhabY
reviewed
Oct 2, 2026
EhabY
left a comment
EhabY
left a comment
Collaborator
There was a problem hiding this comment.
Overall looks good to me, well-tested and simple change but I am unsure if we should terminate the connection instead of just flushing the buffer
…ffer flush The buffer flush is a logging concern, not part of the telemetry event, so the EVENTS.md entry only describes when the event fires and its attributes.
Reaching the unreachable threshold only flushes the buffer; the loop keeps retrying at the backoff cap and never gives up, so a socket recovers after a sleep or long outage. The onConnectionFailure doc still said it fires only on terminal failures, and the warning read like the socket gave up. Reword both, note it in EVENTS.md and the changelog, and pin the behavior with a five-minute outage test that stays in AWAITING_RETRY without re-flushing and then reconnects.
The 100ms used for the backoff options and every timer advance was the same value; a BACKOFF_MS constant makes it clear each advance is one failed attempt.
setupUnreachable hand-rolled a failing factory and called ReconnectingWebSocket.create directly. Forward the backoff and jitter options through FactoryOptions/fromFactory and build it on createReconnectingWebSocketWithErrorControl, toggling failures with setFactoryError.
Four tests repeated startOutage() plus N-1 failed attempts. A failUntilFlush() helper replaces the loop; the threshold test keeps its explicit loop because it asserts nothing flushes before N.
The event's route and attempts are covered by the WebSocketTelemetry unit test, so the reconnecting test just checks the socket emits it at the flush.
aqandrew
enabled auto-merge (squash)
October 7, 2026 19:40
EhabY
approved these changes
Oct 7, 2026
EhabY
disabled auto-merge
October 7, 2026 19:52
"unreachable" is only passed to onConnectionFailure and never dispatched, so it can't appear as a telemetry reason, yet EVENTS.md listed it as one. Add a ConnectionFailureReason union (ConnectionStateReason | "unreachable") for the failure callback in ReconnectingWebSocket and CoderApi, and drop it from the state reasons and the EVENTS.md list.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1112 (deferred from DEVEX-669 / #1100).
Problem
When a server is simply unreachable, the reconnecting WebSocket keeps retrying with backoff and never reaches a terminal failure reason, so
BufferingLoggeris never auto-flushed. A user reproducing a "can't connect / hangs on connecting" problem gets no buffered detail in their support bundle, even though plenty of below-level context was captured.What this does
ReconnectingWebSocket(#consecutiveConnectFailures), incremented once per scheduled retry (both failure paths funnel throughscheduleReconnect, and the initial-connect failure counts too).MAX_RECONNECT_FAILURES_BEFORE_FLUSH = 6, it flushes the buffer once viaonConnectionFailure("unreachable", route), logs awarnbreadcrumb, and emits a newconnection.unreachabletelemetry event.=== Ncheck fires exactly once; a successfulopen(or a user-initiated resume fromDISCONNECTED) resets the counter, so a later outage flushes again."unreachable"toConnectionStateReasonand documentsconnection.unreachableinEVENTS.md, so the reason is a real, queryable signal rather than a phantom union value.## Unreleased.Why 6
With the default backoff (250ms doubling to a 30s cap), the 6th attempt lands after ~15s of retrying: past a transient blip of one or two retries, before the 30s cap, and before a user reproducing a hang would typically give up. It's a module constant for now (easy to tune in review).
Testing
pnpm format:check,pnpm typecheck,pnpm lintclean.reconnectingWebSockettests: flushes once at N with theunreachablereason; not before N; no re-flush while still stuck; resets after a successful open so a later outage flushes again; a transient outage that recovers before N never flushes; andconnection.unreachableis emitted once. Plus a focusedWebSocketTelemetry.unreachableunit test.Implementation plan & decisions
Recommendations settled on the issue's open questions
N = 6, module constant (not a setting). ~15s of retrying with the default backoff; deterministic and testable; time-based rejected since backoff already caps at 30s (YAGNI).#consecutiveConnectFailuresonReconnectingWebSocket, incremented at the top ofscheduleReconnect; reset in theopenhandler and the DISCONNECTED-resume block.N(=== N); increments by 1 so it fires once per episode; a successful open resets it."unreachable"reason (flush keyunreachable <route>), added toConnectionStateReasonandEVENTS.md, and surfaced as a realconnection.unreachabletelemetry event so it isn't a phantom union value.Changes
src/websocket/reconnectingWebSocket.ts— constant, counter field, flush-at-N inscheduleReconnect, resets on open/resume.src/instrumentation/websocket.ts—"unreachable"reason +unreachable(route, attempts)emittingconnection.unreachable.src/instrumentation/EVENTS.md,CHANGELOG.md— docs.test/unit/websocket/reconnectingWebSocket.test.tsandtest/unit/instrumentation/websocket.test.ts.🤖 Generated with Coder Agents on behalf of @aqandrew.